Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a PSR-3 logger adapter and make it injectable #18200

Merged
merged 1 commit into from
Apr 8, 2020
Merged

Add a PSR-3 logger adapter and make it injectable #18200

merged 1 commit into from
Apr 8, 2020

Conversation

ChristophWurst
Copy link
Member

Stumbled over https://www.php-fig.org/psr/psr-3/ because I'm writing a php library that I would like to make Nextcloud-agnostic. To be able to use a logger in the library, I chose to use the PSR-3 standard and inject a tiny adapter.

I figured it would make sense to have this in our code base. This smoothens the integration of 3rd party libraries.

Adding the interface as base of our ILogger is not possible as we have string type hints on the messages, whereas the PSR3 interface only has documented types.

@ChristophWurst ChristophWurst self-assigned this Dec 3, 2019
@danopz
Copy link

danopz commented Dec 8, 2019

PHP-FIG is currently working on adding types to existing PSRs

php-fig/fig-standards#1195
https://www.php-fig.org/blog/2019/10/upgrading-psr-interfaces/

Don't know if there is any plan when it will be there, just wanted to mention that

@ChristophWurst
Copy link
Member Author

PHP-FIG is currently working on adding types to existing PSRs

Thanks a lot for this info. I wasn't aware of that. Then let's wait a bit until this has settled.

@ChristophWurst ChristophWurst added this to the Nextcloud 19 milestone Dec 9, 2019
@ChristophWurst ChristophWurst mentioned this pull request Dec 13, 2019
Closed
@ChristophWurst
Copy link
Member Author

Still no 2.0 🤷‍♂️

@rullzer
Copy link
Member

rullzer commented Feb 7, 2020

Still no 2.0 man_shrugging

do we want to wait?
Or just continue and thighten it later?

@ChristophWurst

This comment has been minimized.

Sign in to view
@ChristophWurst
Copy link
Member Author

We can have types in the implementation already (thanks to php7.2)

Errrrr actually we can't. You can have a broader (super) type as hint, but you can't put string on a method parameter when the interface has no type.

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 7, 2020
@ChristophWurst ChristophWurst marked this pull request as ready for review February 7, 2020 09:13
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Feb 7, 2020
@ChristophWurst

This comment has been minimized.

Sign in to view
@nickvergessen
Copy link
Member

Yeah, I was about to say that. our logger is supposed to be compatible. So I'd say we either remove the type hints for now, or we wait until PSR added them?

@ChristophWurst

This comment has been minimized.

Sign in to view
@ChristophWurst
Add a PSR-3 logger adapter and make it injectable
1b4380a 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Copy link
Member Author

Still no 2.0, so this won't be typed right now. But we can tighten later as you said.

Let's do this :)

@rullzer rullzer mentioned this pull request Apr 4, 2020
Merged
80 tasks
nickvergessen
nickvergessen approved these changes Apr 8, 2020
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me and seems to be additional on top?
Should we ask people to use that instead?

@ChristophWurst
Copy link
Member Author

Fine by me and seems to be additional on top?

Yes, right now our logging stays untouched.

Should we ask people to use that instead?

Let's maybe wait a bit for the typed interface. I hope this lands soon-ish. Then we can advertise the PSR logger more.

@ChristophWurst ChristophWurst merged commit 547ba64 into master Apr 8, 2020
@ChristophWurst ChristophWurst deleted the enhancement/psr-logger branch April 8, 2020 09:49
@ChristophWurst ChristophWurst removed the pending documentation This pull request needs an associated documentation update label Apr 10, 2020
@ChristophWurst ChristophWurst mentioned this pull request Apr 10, 2020
Merged
@ChristophWurst ChristophWurst mentioned this pull request Feb 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rullzer rullzer rullzer approved these changes

@nickvergessen nickvergessen nickvergessen approved these changes

@kesselb kesselb Awaiting requested review from kesselb

@gary-kim gary-kim Awaiting requested review from gary-kim

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees

@ChristophWurst ChristophWurst

Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Milestone
Nextcloud 19
Development

Successfully merging this pull request may close these issues.
4 participants
@ChristophWurst @danopz @rullzer @nickvergessen